Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR: Prepare Project for First Release #2

Merged
merged 6 commits into from
Jan 17, 2025
Merged

Conversation

KelSolaar
Copy link
Member

Summary

This PR prepares the project for a first release:

  • The latest colour check configuration has been applied.
  • Typing annotations have been extended.
  • Documentation and docstrings have been updated.
  • Some breathing/line breaks have been added to the code.
  • colour_clf_io.ExponentStyle, colour_clf_io.LogStyle and colour_clf_io.RangeStyle enumerations were moved to the colour_clf_io.values module.
  • Ensured that the project builds.
  • Ensured that the docs are building: https://colour-clf-io.readthedocs.io/en/latest/
  • Updated the Github templates.

A few notes collected whilst doing this:

  • A few places should have default values, e.g., colour_clf_io.SOPNode.slope but we do not set any.
  • colour_clf_io.parsing.child_elements returning str is a major pain for type checking, we should look at a way of making sure that contextually, it only returns lxml.etree._Element | None. I peppered the code with Pyright pragmas for now.
  • Should colour_clf_io.ProcessNode.description be a list[str] like colour_clf_io.ProcessList.description?

Preflight

Code Style and Quality

  • Unit tests have been implemented and passed.
  • Pyright static checking has been run and passed.
  • Pre-commit hooks have been run and passed.

Documentation

  • New features are documented along with examples if relevant.
  • The documentation is Sphinx and numpydoc compliant.

Copy link
Member

@MichaelMauderer MichaelMauderer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the upgrade and greatly expanded documentation!

colour_clf_io/process_nodes.py Outdated Show resolved Hide resolved
self.assertEqual(len(clf_data.process_nodes), 3)

assert clf_data is not None
assert clf_data.description == ["Conversion from linear ACES2065-1 to ACEScct"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity: what is the benefit of the bare assert over the assertEqual?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None beyond removing the unittest module usage like we did for colour et al. a few years ago.

@MichaelMauderer
Copy link
Member

A few places should have default values, e.g., colour_clf_io.SOPNode.slope but we do not set any.

Yes, I will add that once this PR is merged.

colour_clf_io.parsing.child_elements returning str is a major pain for type checking, we should look at a way of making sure that contextually, it only returns lxml.etree._Element | None. I peppered the code with Pyright pragmas for now.
I think the issue here was the usage of the //text() xpath_functions to get the text directly through the xml.xpath call.

We might be able to refactor that to only extract the element first and then the text further up the call stack.

Should colour_clf_io.ProcessNode.description be a list[str] like colour_clf_io.ProcessList.description?

Yes! Will do that also when this PR is merged.

@KelSolaar KelSolaar merged commit a38402a into develop Jan 17, 2025
28 of 29 checks passed
@KelSolaar KelSolaar deleted the feature/varnish branch January 17, 2025 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants